Feature/gfdlmpv3#17
Conversation
…weather-model into feature/gfdlmpv3
…r-model into feature/gfdlmpv3
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NickSzapiro-NOAA, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates the GFDLMP Version 3.0 (TEMPO) physics scheme, enhancing the model's capabilities with advanced aerosol and hail parameterizations. It also updates the MOM6 ocean model interface and significantly expands the build and testing environment by adding support for the 'Ursa' HPC platform and updating numerous core libraries and compiler configurations. These changes aim to modernize the codebase, improve model accuracy, and broaden platform compatibility.
Highlights
- New Physics Scheme Integration: Introduced GFDLMP Version 3.0 (TEMPO) physics scheme, including new namelist inputs, field tables, and diagnostic tables. This integrates support for aerosol and hail parameterizations within the new scheme.
- MOM6 Ocean Model Updates: Updated the MOM6 ocean model interface, incorporating new Fortran source files for remapping and diagnostics, and reorganizing the 'MOM_generic_tracer' module within the GFDL_ocean_BGC external configuration.
- HPC Platform Expansion: Added 'Ursa' as a new supported HPC platform, which involved creating new CMake configuration files, modulefiles, and updating the regression testing infrastructure to accommodate this new machine.
- Build Environment Modernization: Performed broad updates to various third-party library versions (e.g., ESMF 8.8.0, FMS 2024.02, HDF5 1.14.3, ParallelIO 2.6.2, MAPL 2.53.4) and compiler configurations across multiple HPC platforms, ensuring compatibility and leveraging newer toolchain features.
- Regression Test Suite Refresh: The regression test suite was updated with a new baseline date (20250701) and includes new tests for the integrated GFDLMPv3 and TEMPO physics schemes, along with adjustments for the newly supported Ursa platform, all tests passed successfully.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates, primarily adding support for a new machine platform, ursa, and upgrading numerous dependencies and libraries, including ESMF. It also incorporates new physics options like GFDLMPv3 and TEMPO. The changes span across build configurations, module files for environment setup, and testing scripts. The review identified some critical issues in the module files related to compiler environment variables that could cause build failures. There are also some suggestions for improving script robustness and maintainability by removing debug statements and duplicated code.
| find_package(NetCDF 4.7.4 REQUIRED C Fortran) | ||
| find_package(ESMF 8.3.0 MODULE REQUIRED) | ||
| find_package(ESMF 8.8.0 MODULE REQUIRED) | ||
| #add_library(ESMF::ESMF ALIAS esmf) |
There was a problem hiding this comment.
This commented-out line should be removed if it's no longer necessary. Leaving dead code can be confusing for future maintenance and clutters the file.
| PYTHON_VER="python" | ||
| fi | ||
|
|
||
| echo "using ursa py" |
There was a problem hiding this comment.
This echo statement appears to be for debugging purposes. It should be removed before merging to keep the output clean.
| if [[ ${MACHINE_ID} == "ursa" ]]; then | ||
| PYTHON_VER="python3" | ||
| else | ||
| PYTHON_VER="python" | ||
| fi |
There was a problem hiding this comment.
This logic for determining PYTHON_VER is duplicated from tests-dev/ufs_test.sh. To avoid duplication and ensure consistency, this logic should be centralized. Consider defining and exporting PYTHON_VER in a common setup script that is sourced by both ufs_test.sh and any other script that needs it.
| rm qr_acr_qsV2.dat | ||
| rm qr_acr_qgV2.dat | ||
| rm freezeH2O.dat |
There was a problem hiding this comment.
Using rm without the -f flag can cause the script to fail if the files do not exist. It's safer to use rm -f to suppress errors for non-existent files and make the script more robust.
rm -f qr_acr_qsV2.dat
rm -f qr_acr_qgV2.dat
rm -f freezeH2O.dat
There was a problem hiding this comment.
Code Review
This pull request integrates the GFDLMPv3 microphysics scheme and adds support for the Ursa platform. The changes include updates to build configurations, module files, and testing infrastructure. The review focuses on improving code clarity, maintainability, and robustness.
| find_package(NetCDF 4.7.4 REQUIRED C Fortran) | ||
| find_package(ESMF 8.3.0 MODULE REQUIRED) | ||
| find_package(ESMF 8.8.0 MODULE REQUIRED) | ||
| #add_library(ESMF::ESMF ALIAS esmf) |
There was a problem hiding this comment.
This line is commented out. It would be beneficial to either remove it or provide a comment explaining its purpose and under what circumstances it should be uncommented. This enhances code clarity and maintainability.
| MOM6/src/ALE/Recon1d_EMPLM_CWK.F90 | ||
| MOM6/src/ALE/Recon1d_EMPLM_WA.F90 | ||
| MOM6/src/ALE/Recon1d_EMPLM_WA_poly.F90 | ||
| MOM6/src/ALE/Recon1d_EPPM_CWK.F90 | ||
| MOM6/src/ALE/Recon1d_MPLM_CWK.F90 | ||
| MOM6/src/ALE/Recon1d_MPLM_WA.F90 | ||
| MOM6/src/ALE/Recon1d_MPLM_WA_poly.F90 | ||
| MOM6/src/ALE/Recon1d_PCM.F90 | ||
| MOM6/src/ALE/Recon1d_PLM_CW.F90 | ||
| MOM6/src/ALE/Recon1d_PLM_CWK.F90 | ||
| MOM6/src/ALE/Recon1d_PLM_hybgen.F90 | ||
| MOM6/src/ALE/Recon1d_PPM_CW.F90 | ||
| MOM6/src/ALE/Recon1d_PPM_CWK.F90 | ||
| MOM6/src/ALE/Recon1d_PPM_H4_2018.F90 | ||
| MOM6/src/ALE/Recon1d_PPM_H4_2019.F90 | ||
| MOM6/src/ALE/Recon1d_PPM_hybgen.F90 | ||
| MOM6/src/ALE/Recon1d_type.F90 |
There was a problem hiding this comment.
Consider adding a comment block above these new entries describing what Recon1d refers to, and what these files generally do. This will help improve the code's readability and make it easier for others to understand the purpose of these files at a glance.
| {["libpng"] = "1.6.37"}, | ||
| {["hdf5"] = "1.14.0"}, | ||
| {["hdf5"] = "1.14.3"}, | ||
| {["netcdf-c"] = "4.9.2"}, | ||
| {["netcdf-fortran"] = "4.6.1"}, | ||
| {["parallelio"] = "2.5.10"}, | ||
| {["esmf"] = "8.6.0"}, | ||
| {["fms"] = "2024.01"}, | ||
| {["parallelio"] = "2.6.2"}, | ||
| {["esmf"] = "8.8.0"}, | ||
| {["fms"] = "2024.02"}, | ||
| {["bacio"] = "2.4.1"}, | ||
| {["crtm"] = "2.4.0"}, | ||
| {["crtm"] = "2.4.0.1"}, | ||
| {["g2"] = "3.5.1"}, | ||
| {["g2tmpl"] = "1.13.0"}, | ||
| {["ip"] = "4.3.0"}, | ||
| {["ip"] = "5.1.0"}, | ||
| {["sp"] = "2.5.0"}, | ||
| {["w3emc"] = "2.10.0"}, | ||
| {["gftl-shared"] = "1.6.1"}, | ||
| {["mapl"] = "2.40.3-esmf-8.6.0"}, | ||
| {["gftl-shared"] = "1.9.0"}, | ||
| {["mapl"] = "2.53.4-esmf-8.8.0"}, | ||
| {["scotch"] = "7.0.4"}, |
There was a problem hiding this comment.
These library version updates are significant. Consider adding a comment block summarizing the key changes or reasons for these updates, especially if they address specific bugs or performance issues. This provides valuable context for future maintainers.
|
|
||
| nccmp_ver=os.getenv("nccmp_ver") or "1.9.0.1" |
There was a problem hiding this comment.
Consider using mpiicx and mpiicpx instead of mpiicc and mpiicpc respectively, as the former are the recommended compilers for Intel oneAPI. Also, setting I_MPI_CC, I_MPI_CXX, and I_MPI_F90 will ensure that the correct compilers are used by MPI.
setenv("CC", "mpiicx")
setenv("CXX", "mpiicpx")
setenv("FC", "mpiifort")
setenv("I_MPI_CC", "icx")
setenv("I_MPI_CXX", "icpx")
setenv("I_MPI_F90", "ifort")
9 participants
Commit Queue Requirements:
Description:
Commit Message:
Priority:
Git Tracking
UFSWM:
Sub component Pull Requests:
UFSWM Blocking Dependencies:
Documentation:
Changes
Regression Test Changes (Please commit test_changes.list):
Input data Changes:
Library Changes/Upgrades:
Testing Log: